-
Notifications
You must be signed in to change notification settings - Fork 4
Add spec test functionality #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
pymongoexplain/commands.py
Outdated
@@ -39,6 +39,8 @@ def command_name(self): | |||
def get_SON(self): | |||
cmd = SON([(self.command_name, self.collection)]) | |||
cmd.update(self.command_document) | |||
if self.command_document == {}: | |||
return {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were a few cases where the filter was {} and the expected command payload in that case was also {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not be possible for a command to be empty. Could you post those cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in the following cases:
SON([('find', 'test_find_allowdiskuse'), ('filter', {})])
{}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which spec test is this specifically? Can you post the entire test failure output?
pymongoexplain/commands.py
Outdated
@@ -51,8 +53,13 @@ def __init__(self, collection: Collection, filter, update, | |||
value = kwargs[key] | |||
if key == "bypass_document_validation": | |||
return_document[key] = value | |||
elif key == "hint": | |||
return_document["updates"][0]["hint"] = value if \ | |||
isinstance(value, str) else SON(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not have the same behavior as Pymongo. Check out how the Collection class handles the "hint" option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused about this comment. This matches the output produced by Pymongo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check out _index_document and where that method is used: https://github.com/mongodb/mongo-python-driver/blob/956ce3d4b0edfa9c1d946109db743f82ed0bfc0a/pymongo/helpers.py#L79
We also need to use _index_document for sort
.
pymongoexplain/commands.py
Outdated
else: | ||
return_document["updates"][0][key] = value | ||
if return_document["updates"][0].get("hint", True) == {}: | ||
del return_document["updates"][0]["hint"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding and then removing "hint" how about we only add it if needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing it to only be added when needed results in 1 test failing, not sure how to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you post those failures here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran 106 tests in 11.719s
FAILED (failures=1, skipped=42)
SON([('update', 'test'), ('updates', [{'q': {'_id': 1}, 'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}], 'upsert': False, 'hint': {}, 'multi': False}])])
SON([('update', 'test'), ('ordered', True), ('lsid', {'id': Binary(b'\xca\xa9L\x9d\x84\x86L\xc7\x9f\xfa\x8cE4\xa8Kq', 4)}), ('txnNumber', 1), ('$clusterTime', {'clusterTime': Timestamp(1595449666, 16), 'signature': {'hash': b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00', 'keyId': 0}}), ('$db', 'crud-tests'), ('$readPreference', {'mode': 'primary'}), ('updates', [SON([('q', {'_id': 1}), ('u', [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}]), ('multi', False), ('upsert', False)])])])
[{'multi': False,
'q': {'_id': 1},
'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}],
'upsert': False}] != [{'hint': {},
'multi': False,
'q': {'_id': 1},
'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}],
'upsert': False}]
@@ -81,7 +88,12 @@ def __init__(self, collection: Collection, pipeline, session, | |||
super().__init__(collection.name) | |||
self.command_document = {"pipeline": pipeline, "cursor": cursor_options} | |||
for key, value in kwargs.items(): | |||
self.command_document[key] = value | |||
if key == "batchSize": | |||
if value == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check for None too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None values are already removed in the converting to snakecase step
pymongoexplain/commands.py
Outdated
@@ -110,18 +122,30 @@ def __init__(self, collection: Collection, | |||
super().__init__(collection.name) | |||
for key, value in kwargs.items(): | |||
self.command_document[key] = value | |||
if self.command_document["filter"] == {}: | |||
self.command_document = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? What about all the other find command options? Like:
coll.find({}, limit=10)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the only way to make all the tests pass, so I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test for coll.find({}, limit=10)
to ensure that the limit field (and other command args) are not removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test.
pymongoexplain/commands.py
Outdated
if "replacement" in self.command_document.keys(): | ||
self.command_document["update"] = self.command_document[ | ||
"replacement"] | ||
del self.command_document["replacement"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more readable to construct the command correctly from the start rather than constructing an invalid command and then patching it up after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confused about this comment. The command is being constructed in this step. The kwargs value of replacement has to be patched in that way otherwise it will simply not produce the correct output. If we did not put this code there, then it would have to be duplicated multiple times in explainable_collection.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are certain processing steps that must be followed to match pymongo's output, and most of them seem rather arbitrary so the only way for me to verify is to match my code to the test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm trying to get at is that it's confusing to add the "replacement" field to command_document and then immediately remove it. Instead I think this class should handle all the findAndModify specific options while we constructing the command. Like this:
command_document = {
"findAndModify": collection_name,
"query": filter,
"new": return_document,
"update": replacement,
}
collation = validate_collation_or_none(kwargs.pop('collation', None))
if collation is not None:
cmd["collation"] = collation
if projection is not None:
cmd["fields"] = helpers._fields_list_to_dict(projection,
"projection")
if sort is not None:
cmd["sort"] = helpers._index_document(sort)
if upsert is not None:
common.validate_boolean("upsert", upsert)
cmd["upsert"] = upsert
if hint is not None:
if not isinstance(hint, str):
hint = helpers._index_document(hint)
cmd['hint'] = hint
if array_filters is not None:
cmd["arrayFilters"] = array_filters
This approach exposes numerous bugs in the current approach. Sometimes an argument name needs validation (like a type assertion), sometimes it needs to be transformed (like collation
or hint
), and sometimes the python argument name is different from the command argument name (like replacement
-> update
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the deleting statements and tried to condense all the logic down into the for loops.
def __init__(self, collection): | ||
self.collection = collection | ||
def __init__(self, collection_object): | ||
self.collection_object = collection_object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "collection" is a better name. "collection_object" is a bit verbose for Python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In these past few commits I have tried to condense down the logic and get rid of some of the delete statements so that it's easier to follow the construction of commands.
pymongoexplain/commands.py
Outdated
else: | ||
return_document["updates"][0][key] = value | ||
if return_document["updates"][0].get("hint", True) == {}: | ||
del return_document["updates"][0]["hint"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran 106 tests in 11.719s
FAILED (failures=1, skipped=42)
SON([('update', 'test'), ('updates', [{'q': {'_id': 1}, 'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}], 'upsert': False, 'hint': {}, 'multi': False}])])
SON([('update', 'test'), ('ordered', True), ('lsid', {'id': Binary(b'\xca\xa9L\x9d\x84\x86L\xc7\x9f\xfa\x8cE4\xa8Kq', 4)}), ('txnNumber', 1), ('$clusterTime', {'clusterTime': Timestamp(1595449666, 16), 'signature': {'hash': b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00', 'keyId': 0}}), ('$db', 'crud-tests'), ('$readPreference', {'mode': 'primary'}), ('updates', [SON([('q', {'_id': 1}), ('u', [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}]), ('multi', False), ('upsert', False)])])])
[{'multi': False,
'q': {'_id': 1},
'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}],
'upsert': False}] != [{'hint': {},
'multi': False,
'q': {'_id': 1},
'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}],
'upsert': False}]
pymongoexplain/commands.py
Outdated
if "replacement" in self.command_document.keys(): | ||
self.command_document["update"] = self.command_document[ | ||
"replacement"] | ||
del self.command_document["replacement"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the deleting statements and tried to condense all the logic down into the for loops.
pymongoexplain/commands.py
Outdated
@@ -110,18 +122,30 @@ def __init__(self, collection: Collection, | |||
super().__init__(collection.name) | |||
for key, value in kwargs.items(): | |||
self.command_document[key] = value | |||
if self.command_document["filter"] == {}: | |||
self.command_document = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test.
pymongoexplain/commands.py
Outdated
if key == "bypass_document_validation": | ||
return_document[key] = value | ||
elif key == "hint": | ||
if value is not {} and value is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the {}
check here. It's invalid for hint to be {}
:
>>>> client.t.t.update_one({}, {'$set': {'a':1}}, hint={})
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/shane/git/mongo-python-driver/pymongo/collection.py", line 1024, in update_one
hint=hint, session=session),
File "/Users/shane/git/mongo-python-driver/pymongo/collection.py", line 870, in _update_retryable
_update, session)
File "/Users/shane/git/mongo-python-driver/pymongo/mongo_client.py", line 1497, in _retryable_write
return self._retry_with_session(retryable, func, s, None)
File "/Users/shane/git/mongo-python-driver/pymongo/mongo_client.py", line 1383, in _retry_with_session
return self._retry_internal(retryable, func, session, bulk)
File "/Users/shane/git/mongo-python-driver/pymongo/mongo_client.py", line 1415, in _retry_internal
return func(session, sock_info, retryable)
File "/Users/shane/git/mongo-python-driver/pymongo/collection.py", line 866, in _update
retryable_write=retryable_write)
File "/Users/shane/git/mongo-python-driver/pymongo/collection.py", line 806, in _update
hint = helpers._index_document(hint)
File "/Users/shane/git/mongo-python-driver/pymongo/helpers.py", line 87, in _index_document
"mean %r?" % list(iteritems(index_list)))
TypeError: passing a dict to sort/create_index/hint is not allowed - use a list of tuples instead. did you mean []?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
class ExplainCollection(): | ||
def __init__(self, collection): | ||
self.collection = collection | ||
self.last_cmd_payload = None | ||
|
||
def _explain_command(self, command): | ||
command_son = command.get_SON() | ||
if command_son == {}: | ||
self.last_cmd_payload = {} | ||
return {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed. It's impossible for a command to be empty ({}
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pymongoexplain/commands.py
Outdated
@@ -108,6 +121,8 @@ class FindCommand(BaseCommand): | |||
def __init__(self, collection: Collection, | |||
kwargs): | |||
super().__init__(collection.name) | |||
if kwargs["filter"] == {}: | |||
self.command_document = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a bug (or it's masking a bug). Let's remove this code and address the failing test in a different way. Please post the test failure here.
For reference this is how pymongo generates a find command:
https://github.com/mongodb/mongo-python-driver/blob/3.11.0rc0/pymongo/message.py#L181-L217
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought that might be a bug because it doesn't really make sense. Removing that doesn't seem to cause any additional test failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The three different tests that fail are below, they are in the format of <the command we constructed>, <the command that was expected>
SON([('find', 'test_find_allowdiskuse'), ('filter', {})])
{}
SON([('find', 'test_find_allowdiskuse'), ('allowDiskUse', True), ('filter', {})])
{}
SON([('find', 'test_find_allowdiskuse'), ('allowDiskUse', False), ('filter', {})])
{}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you post the entire failed test output though? Like this:
FAIL: test_keyword_arg_defaults (test.test_client.ClientUnitTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/shane/git/mongo-python-driver/test/test_client.py", line 128, in test_keyword_arg_defaults
self.assertEqual(20.1, pool_opts.connect_timeout)
AssertionError: 20.1 != 20.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SON([('find', 'test_find_allowdiskuse'), ('filter', {})])
{}
Error
Traceback (most recent call last):
File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
yield
File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 676, in run
self._callTestMethod(testMethod)
File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
method()
File "/Users/julius/pymongoexplain/test/__init__.py", line 442, in wrap
return f(*args, **kwargs)
File "/Users/julius/pymongoexplain/test/test_crud_v2.py", line 62, in run_scenario
self.run_scenario(scenario_def, test)
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 615, in run_scenario
self.run_test_ops(sessions, collection, test)
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 525, in run_test_ops
self.run_operations(sessions, collection, test['operations'])
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 431, in run_operations
result = self.run_operation(sessions, collection, op.copy())
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 374, in run_operation
self._compare_command_dicts(wrapped_collection.last_cmd_payload,
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 269, in _compare_command_dicts
self.assertEqual(ours[key], theirs[key])
KeyError: 'find
SON([('find', 'test_find_allowdiskuse'), ('allowDiskUse', True), ('filter', {})])
{}
Error
Traceback (most recent call last):
File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
yield
File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 676, in run
self._callTestMethod(testMethod)
File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
method()
File "/Users/julius/pymongoexplain/test/__init__.py", line 442, in wrap
return f(*args, **kwargs)
File "/Users/julius/pymongoexplain/test/test_crud_v2.py", line 62, in run_scenario
self.run_scenario(scenario_def, test)
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 615, in run_scenario
self.run_test_ops(sessions, collection, test)
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 525, in run_test_ops
self.run_operations(sessions, collection, test['operations'])
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 431, in run_operations
result = self.run_operation(sessions, collection, op.copy())
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 374, in run_operation
self._compare_command_dicts(wrapped_collection.last_cmd_payload,
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 269, in _compare_command_dicts
self.assertEqual(ours[key], theirs[key])
KeyError: 'find'
SON([('find', 'test_find_allowdiskuse'), ('allowDiskUse', False), ('filter', {})])
{}
Error
Traceback (most recent call last):
File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
yield
File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 676, in run
self._callTestMethod(testMethod)
File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
method()
File "/Users/julius/pymongoexplain/test/__init__.py", line 442, in wrap
return f(*args, **kwargs)
File "/Users/julius/pymongoexplain/test/test_crud_v2.py", line 62, in run_scenario
self.run_scenario(scenario_def, test)
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 615, in run_scenario
self.run_test_ops(sessions, collection, test)
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 525, in run_test_ops
self.run_operations(sessions, collection, test['operations'])
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 431, in run_operations
result = self.run_operation(sessions, collection, op.copy())
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 374, in run_operation
self._compare_command_dicts(wrapped_collection.last_cmd_payload,
File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 269, in _compare_command_dicts
self.assertEqual(ours[key], theirs[key])
KeyError: 'find'
pymongoexplain/commands.py
Outdated
"updates":[{"q": filter, "u": update}] | ||
}) | ||
if upsert is not None: | ||
self.command_document["updates"][0]["upsert"] = upsert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest refactoring this to:
update_doc = {"q": filter, "u": update}
self.command_document["updates"] = [update_doc]
if upsert is not None:
update_doc["upsert"] = upsert
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this change. Did it get lost?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored it incorrectly, it is done now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think this is complete. We can replace self.command_document["updates"][0]
with update_doc
now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pymongoexplain/commands.py
Outdated
self.command_document = {"pipeline": pipeline, "cursor": cursor_options} | ||
kwargs): | ||
|
||
super().__init__(collection.name, kwargs.get("collation", None)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be using kwargs.pop("collation", None)
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -76,7 +75,7 @@ def count_documents(self, filter: Document, session=None, | |||
**kwargs): | |||
|
|||
command = AggregateCommand(self.collection, [{'$match': filter}, | |||
{'$group': {'n': {'$sum': 1}, '_id': 1}}], | |||
{'$group': {'n': {'$sum': 1}, '_id': 1}}], | |||
session, {}, kwargs, | |||
exclude_keys=filter.keys()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is exclude_keys
?
pymongoexplain/commands.py
Outdated
@@ -63,10 +92,11 @@ def command_name(self): | |||
class DistinctCommand(BaseCommand): | |||
def __init__(self, collection: Collection, key, filter, session, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do some of these classes accept a session
but some don't? I think they should all be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
pymongoexplain/commands.py
Outdated
super().__init__(collection.name, kwargs.get("collation", None)) | ||
self.command_document.update({"key": key, "query": filter}) | ||
if kwargs.get("read_concern", None) is not None: | ||
self.command_document["read_concern"] = kwargs["read_concern"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
read_concern
is not an option for distinct so we can remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still some lingering correctness issues here. For example, the sort
and projection
arguments need to be converted like pymongo does: https://github.com/ShaneHarvey/mongo-python-driver/blob/6932d25639e35a53a67a7960be0603720ce48b00/pymongo/collection.py#L2920-L2924
For example:
>>> client.t.t.find_one(projection=['a', 'b.c'])
>>> client.t.t.find_one(projection={'a': 1, 'b.c': 1})
Please audit these arguments and add tests as you fix these issues to ensure ExplainableCollection works correctly in these cases.
pymongoexplain/commands.py
Outdated
super().__init__(collection.name) | ||
self.command_document = {"query": filter} | ||
def __init__(self, collection: Collection, filter, kwargs, | ||
sxclude_keys=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove sxclude_keys
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
i, exclude_keys=exclude_keys) for i in d[key]] | ||
elif isinstance(d[key], dict): | ||
ret[new_key] = convert_to_camelcase(d[key], | ||
exclude_keys=exclude_keys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I think we can remove the exclude_keys
argument now too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -149,29 +146,30 @@ def find_one_and_delete(self, filter: Document, projection: list = None, | |||
kwargs) | |||
return self._explain_command(command) | |||
|
|||
def find_one_and_replace(self, filter: Document, replacement: Document, | |||
def find_one_and_replace(self, filter: Document, update: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This argument should still be called "replacement", not "update". It needs to match Pymongo's find_one_and_replace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pymongoexplain/commands.py
Outdated
for key, value in kwargs.items(): | ||
self.command_document[key] = value | ||
if key == "update" and kwargs.get("replacement", None) is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove all the "replacement" references here because we never pass "replacement" through kwargs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replacement is passed through kwargs for find_one_and_replace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I was confused. Fixed it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good now but I am concerned that there might still be lingering bugs in fields that are not tested by our SPEC test suite. @ShaneHarvey do you have any idea if/how we might be able to leverage our PyMongo-native test suite to test such fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please wait for @ShaneHarvey's approval before merging.
pymongoexplain/commands.py
Outdated
"updates":[{"q": filter, "u": update}] | ||
}) | ||
if upsert is not None: | ||
self.command_document["updates"][0]["upsert"] = upsert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think this is complete. We can replace self.command_document["updates"][0]
with update_doc
now.
setup.py
Outdated
tests_require=["pymongo==3.10.1"], | ||
install_requires=['pymongo==3.10.1'], | ||
tests_require=["pymongo==3.11.0rc0"], | ||
install_requires=['pymongo==3.11.0rc0'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be pymongo>=3.11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pymongoexplain/commands.py
Outdated
@@ -20,15 +20,20 @@ | |||
|
|||
from bson.son import SON | |||
from pymongo.collection import Collection | |||
from pymongo.helpers import _index_document, _fields_list_to_dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though we maintain pymongo we can't rely on any private methods (in python names starting with _
mean private). Could you vendor (copy) these two methods instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pymongoexplain/commands.py
Outdated
@@ -19,16 +19,71 @@ | |||
from typing import Union | |||
|
|||
from bson.son import SON | |||
from bson.py3compat import abc, iteritems, string_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
py3compat
is intended as an internal only api within bson. Since we only support python 3 can you remove this and replace each one with the native Python 3-only solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the crud/v1/tests files before merging - AFAICT we are not running these tests so it makes no sense to have these files in the repo.
Also, I have opened #24 as a follow-up for this work (not part of the 1.0 milestone though).
Great job!
No description provided.